Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a crash in pack() when being handed tables with no columns. #8697

Merged
merged 1 commit into from
Jul 13, 2021

Conversation

nvdbaranec
Copy link
Contributor

Also changes the behavior of pack() such that when returning empty data, the metadata_ and gpu_data unique_ptrs are not null, but instead point to empty metadata and rmm::device_buffer objects, respectively. Very mild breakage of the interface.

…so changed the packed_columns struct so that even when returned empty, instead of metadata_ and gpu_data

unique_ptrs being null, they will point to empty objects, making things a little safer.
@nvdbaranec nvdbaranec added bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. breaking Breaking change labels Jul 8, 2021
@nvdbaranec nvdbaranec requested a review from a team as a code owner July 8, 2021 22:47
@codecov
Copy link

codecov bot commented Jul 9, 2021

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.08@8aaf8e6). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.08    #8697   +/-   ##
===============================================
  Coverage                ?   10.63%           
===============================================
  Files                   ?      109           
  Lines                   ?    18659           
  Branches                ?        0           
===============================================
  Hits                    ?     1985           
  Misses                  ?    16674           
  Partials                ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8aaf8e6...cf72fdd. Read the comment docs.

@nvdbaranec nvdbaranec added 5 - DO NOT MERGE Hold off on merging; see PR for details and removed 5 - DO NOT MERGE Hold off on merging; see PR for details labels Jul 9, 2021
@harrism harrism added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Jul 13, 2021
@harrism
Copy link
Member

harrism commented Jul 13, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 7521c3f into rapidsai:branch-21.08 Jul 13, 2021
rapids-bot bot pushed a commit that referenced this pull request Jul 19, 2021
…vice data (#8759)

A few changes in here to resolve some test failures using pack/unpack with DataFrame serialization:

- We now use a `Buffer` in `frames` to represent the device data, so that Dask can correctly perform the necessary DtoD transfers when moving a packed columns object between devices
- With #8697 merged, the results of packing an empty DataFrame will set the pointer to the host data to `NULL`; since Cython cannot make a memoryview from `NULL`, we now check for this condition before making the host data array
- The serialized type is now included in the serialized header

Authors:
  - Charles Blackmon-Luca (https://github.com/charlesbluca)

Approvers:
  - https://github.com/brandon-b-miller
  - Michael Wang (https://github.com/isVoid)

URL: #8759
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge breaking Breaking change bug Something isn't working libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants